fix(http): route error and empty-result messages to stderr#485
fix(http): route error and empty-result messages to stderr#485abhiram304 wants to merge 1 commit intogoogleworkspace:mainfrom
Conversation
🦋 Changeset detectedLatest commit: d3b32d3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the application's output behavior by ensuring that human-readable messages and API error details are directed to Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request correctly routes human-readable messages and HTTP error bodies to stderr, which is a great improvement for consumers of the CLI that pipe stdout to other tools like jq. The changes in gmail/triage.rs and modelarmor.rs are logical and well-implemented. I've added a couple of comments regarding the newly added tests, with suggestions to make them more robust and less prone to giving a false sense of security.
Two violations were breaking pipe-friendly use of the CLI: 1. gmail/triage.rs printed "No messages found…" via println! on both the null-messages and empty-array paths, corrupting stdout for `gws gmail +triage | jq` pipelines. Changed to eprintln!. 2. modelarmor.rs printed the raw API response body unconditionally (before the status check), so on HTTP error the error body leaked to stdout instead of surfacing in the error return. Moved the println! inside the success branch and included the body in the Err message for better diagnostics.
fa34e93 to
d3b32d3
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly routes error and informational messages to stderr instead of stdout, which is a great improvement for pipeline-based workflows. The changes in gmail/triage.rs and modelarmor.rs are well-targeted to fix this issue. I've added a comment with a suggestion to improve testing robustness, pointing out that a new test in modelarmor.rs doesn't fully test the intended error path, which is crucial for ensuring correct error message routing.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #485 +/- ##
==========================================
+ Coverage 67.57% 67.59% +0.02%
==========================================
Files 40 40
Lines 17475 17489 +14
==========================================
+ Hits 11808 11822 +14
Misses 5667 5667 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…bels, auth propagation Component 1 (PR #485): Route triage 'no messages' and modelarmor error bodies to stderr so stdout stays machine-readable. Component 2 (PR #466): Add colored error[variant]: labels to stderr on TTY, respecting NO_COLOR. Replace emoji hint with colorized text. Component 3 (PR #446): Propagate auth errors as GwsError::Auth in calendar, chat, docs, drive, script, sheets helpers instead of silently proceeding unauthenticated. dry-run bypass preserved.
* fix: stderr/output hygiene rollup — diagnostics to stderr, colored labels, auth propagation Component 1 (PR #485): Route triage 'no messages' and modelarmor error bodies to stderr so stdout stays machine-readable. Component 2 (PR #466): Add colored error[variant]: labels to stderr on TTY, respecting NO_COLOR. Replace emoji hint with colorized text. Component 3 (PR #446): Propagate auth errors as GwsError::Auth in calendar, chat, docs, drive, script, sheets helpers instead of silently proceeding unauthenticated. dry-run bypass preserved. * fix: deduplicate accessNotConfigured stderr output Use if/else so that accessNotConfigured errors get the specialized hint guidance instead of redundantly printing both the generic summary and the hint. Non-accessNotConfigured Api errors and all other variants still get the generic error[variant]: summary line. * test: remove misleading model_armor_post error format test model_armor_post function. A proper integration test would require HTTP mocking (e.g. mockito/wiremock) which is out of scope for this PR. * refactor: deduplicate error printing else branches Use early return in accessNotConfigured branch so the generic eprintln! only appears once, eliminating the duplicated else blocks. * security: sanitize error messages before printing to stderr Add sanitize_for_terminal() to strip control characters (ANSI escape sequences, bell, backspace, etc.) from error messages before printing to stderr, preventing terminal escape injection from API responses. Newlines and tabs are preserved for readability. The function is pub(crate) so it can be reused by other modules that print untrusted content to stderr. * fix: sanitize all stderr error output across codebase Apply sanitize_for_terminal() to all 16 remaining eprintln sites that print unsanitized error strings to stderr. This prevents terminal escape sequence injection through error messages. Files updated: - workflows.rs (4 sites) - watch.rs (2 sites) - gmail/mod.rs (3 sites) - executor.rs (1 site) - subscribe.rs (1 site) - token_storage.rs (2 sites) - credential_store.rs (2 sites) - setup.rs (1 site) - generate_skills.rs (1 site) Also fixes clippy: map_err -> inspect_err where closure only logs. --------- Co-authored-by: jpoehnelt-bot <jpoehnelt-bot@users.noreply.github.com>
Summary
gmail/triage.rswas printing"No messages found matching query: {query}"viaprintln!on both the null-messages and empty-array paths — this corrupted stdout forgws gmail +triage | jqstyle pipelines. Changed toeprintln!.modelarmor.rs(model_armor_post) was printing the raw API response body unconditionally before the status check, so on HTTP 4xx/5xx the error body leaked to stdout. Movedprintln!("{text}")inside the success branch and included{text}in theErrmessage so callers get actionable diagnostics without stdout pollution.Test plan
gws gmail +triage --query label:inbox 2>/dev/null | jq .— stdout is valid JSON only, empty-result message appears on stderrcargo test— all tests pass (triage empty-result message test, modelarmor error path test)🤖 Generated with Claude Code